Skip to content

Comments

Add with_error_callback to OutputStreamBuilder#708

Merged
yara-blue merged 7 commits intoRustAudio:masterfrom
Mynd-Group:with_error_callback
Mar 5, 2025
Merged

Add with_error_callback to OutputStreamBuilder#708
yara-blue merged 7 commits intoRustAudio:masterfrom
Mynd-Group:with_error_callback

Conversation

@will3942
Copy link
Contributor

Allows a user to handle a cpal error, allowing them to optionally recreate the stream if desired or take another action (such as relaunching the app).

I wasn't able to do this without using Box - happy to take any suggestions if you can think of a better implementation as I only have very nascent Rust knowledge.

Closes #465

@yara-blue
Copy link
Member

I wasn't able to do this without using Box - happy to take any suggestions if you can think of a better implementation as I only have very nascent Rust knowledge.

make the builder generic over the error callback (you will need to use OutputStreamBuilder<ErrorCallback> instead of Self<ErrorCallback in with_error_callback). There should be no need to make OutputStream generic.

@will3942
Copy link
Contributor Author

I wasn't able to do this without using Box - happy to take any suggestions if you can think of a better implementation as I only have very nascent Rust knowledge.

make the builder generic over the error callback (you will need to use OutputStreamBuilder<ErrorCallback> instead of Self<ErrorCallback in with_error_callback). There should be no need to make OutputStream generic.

I tried that for a while but was unable to get it working with a fallback to the default error callback but will try again next week.

@PetrGlad
Copy link
Collaborator

PetrGlad commented Mar 2, 2025

I think a dyn Fn makes sense here, the callback is not performance-critical.

The problem with opening default stream is that it may try to open stream multiple times. At the same time the call back gets owned at each attempt. See also my comment in open_stream_or_fallback.

@yara-blue
Copy link
Member

I think a dyn Fn makes sense here, the callback is not performance-critical.

I worry about the underflow error (when the source can not deliver samples fast enough to the OS). The a slow error handler could make the problem worse, a single underflow error could cause many more. I do not know if the delay caused by dyn is severe enough to cause that.

@will3942
Copy link
Contributor Author

will3942 commented Mar 4, 2025

I think a dyn Fn makes sense here, the callback is not performance-critical.

I worry about the underflow error (when the source can not deliver samples fast enough to the OS). The a slow error handler could make the problem worse, a single underflow error could cause many more. I do not know if the delay caused by dyn is severe enough to cause that.

@dvdsk I have now implemented this using generics instead without dyn dispatch. Let me know your feedback.

Copy link
Member

@yara-blue yara-blue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, if you want to you could make the example a little more in-depth (see comment) but this is good to merge.

@will3942
Copy link
Contributor Author

will3942 commented Mar 5, 2025

Looks great, if you want to you could make the example a little more in-depth (see comment) but this is good to merge.

All done - feel free to merge once you're happy!

@yara-blue
Copy link
Member

O.o. CI is broken it seems....

well I'll see if I can fix that in a bit. PR looks great, ill try to get CI sorted and then merge this.

@will3942
Copy link
Contributor Author

will3942 commented Mar 5, 2025

O.o. CI is broken it seems....

well I'll see if I can fix that in a bit. PR looks great, ill try to get CI sorted and then merge this.

CI is all fixed - I broke it by forgetting a cargo fmt. Thanks for your help on this PR!

@yara-blue yara-blue merged commit 4cc1a57 into RustAudio:master Mar 5, 2025
17 of 18 checks passed
@PetrGlad
Copy link
Collaborator

PetrGlad commented Mar 9, 2025

I wonder if anyone can simulate/test underflow error. I could not. On my system no matter how slow the source is pipewire/ALSA does not complain (at least not through CPAL).

@PetrGlad
Copy link
Collaborator

PetrGlad commented Mar 9, 2025

On my system (Linux+ALSA+Pipewire). When I stop audio server I get BackendSpecificError instead.
Generally, I do not believe there are any recoverable errors that we get in the callback.

@will3942
Copy link
Contributor Author

will3942 commented Mar 9, 2025

On my system (Linux+ALSA+Pipewire). When I stop audio server I get BackendSpecificError instead.

Generally, I do not believe there are any recoverable errors that we get in the callback.

On CoreAudio/Mac we get the correct error and can destroy & recreate the output stream with a different device.

@PetrGlad
Copy link
Collaborator

PetrGlad commented Mar 9, 2025

@will3942 Are there any recoverable errors you get from this callback?

@will3942
Copy link
Contributor Author

will3942 commented Mar 9, 2025

@will3942 Are there any recoverable errors you get from this callback?

How do you define recoverable? Recover without having to tear down and destroy the resources?

@roderickvd
Copy link
Member

I wonder if anyone can simulate/test underflow error. I could not. On my system no matter how slow the source is pipewire/ALSA does not complain (at least not through CPAL).

One way to do it is to use an RPi with SD card and do a lot of I/O on it, writes in particular.

@PetrGlad
Copy link
Collaborator

@will3942 Yes, by "recoverable" here I mean being able to proceed without re-initializing the stream.

@PetrGlad
Copy link
Collaborator

@roderickvd The output stream can not know where delay comes from. So I believe that just artificially stalling the sample callback should have the same effect.

@will3942
Copy link
Contributor Author

@will3942 Yes, by "recoverable" here I mean being able to proceed without re-initializing the stream.

Not that I'm aware of, no.

In my application code I will be using this to remove and recreate the OutputStream, checking if the device is still available. It would be nice if Rodio could optionally provide this functionality out of the box but it feels like there's quite a few toggles/options/timeouts you'd want to provide the user in determining how that logic is provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow user to get to cpal errors such as device unplugged.

4 participants